Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grass.script: Add separate function for setting location description #3431

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

wenzeslaus
Copy link
Member

This adds a separate function for setting location (aka title) stored in MYNAME file. It includes tests of this function.

This adds a separate function for setting location (aka title) stored in MYNAME file. It includes tests of this function.
@wenzeslaus wenzeslaus added this to the 8.4.0 milestone Feb 16, 2024
@wenzeslaus wenzeslaus self-assigned this Feb 16, 2024
@wenzeslaus
Copy link
Member Author

What is not 100% clear to me is whether the function should be public or protected. I made it protected, but it could be public in grass.script or in grass.grassdb. _create_location_xy looks like a candidate for being public as well.

@github-actions github-actions bot added Python Related code is in Python libraries labels Feb 16, 2024
@echoix
Copy link
Member

echoix commented Feb 16, 2024

I'm not sure if it's totally related, but with the previous PR that changed the grass script env loading, what I observed in my debugging in the last two-three months, is that the namespace is pretty big, and it imports absolutely everything. So one of my wishes, is that one day we could

  1. not have star imports in the __init__.py of the major modules (Only explicit, so even static checkers and IDE could help us)
  2. Not having to import too much for a simple operation

I didn't figure out how it could be non-breaking, since I don't know how the mega poluted namespaces (grass.script for example) is used. Internally, we could adapt to use exactly what we need, and use another dot where to find the functons we need.
What I observed, is that for every small call, that happens to import grass.script, means that almost all python files must be working, and with all the side effects, must also mean that the environment (the setup of the computer, session, env var, installation, mapsets, locations, ie: everything), must be perfectly working in order to just parse through the python files, or call a module description. So it probably has impacts to have everything parsed each time.

All this to say, if you have to have something that is only for a specific use, it be good to place it where it should, and be imported on a needed basis, and not necessarily eagerly added to an __init__.py. Adding them there is like a shortcut to have it available in that namespace, it doesn't prevent us from doing from grass.script import core and core.that_function, instead of having that_function() already available after import grass.script.

@wenzeslaus
Copy link
Member Author

@echoix I generally agree. The major clean up needs to happen only for v9, but we can restructure any time if we spend the extra time ensuring backwards compatibility. I'm happy to have an extended discussion elsewhere.

Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but it probably shouldn't use os.linesep.

@echoix
Copy link
Member

echoix commented Mar 6, 2024

Unrelated, but it probably shouldn't use os.linesep.

From https://docs.python.org/3.12/library/os.html#os.linesep

os.linesep¶
The string used to separate (or, rather, terminate) lines on the current platform. This may be a single character, such as '\n' for POSIX, or multiple characters, for example, '\r\n' for Windows. Do not use os.linesep as a line terminator when writing files opened in text mode (the default); use a single '\n' instead, on all platforms.


Is codecs opened in text mode here?
Oh, and I just thought of this, this might be why so much of the windows tests are failing... but the same note has been there since the python 2.6 docs, the oldest I could go back on the site.

@wenzeslaus wenzeslaus merged commit ac63b38 into OSGeo:main Mar 7, 2024
25 checks passed
@wenzeslaus wenzeslaus deleted the set-loc-name-as-a-function branch March 7, 2024 01:56
@wenzeslaus
Copy link
Member Author

Both os.linesep and codecs can be likely replaced by much simpler code in Python 3, but I'm leaving that for a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants